Read notebooks saved in the nbformat 3 layout - #2612

Review: independent verification of nbformat 3 support

Thanks Chen Yufeiyang (@feiiiiii5) — I reproduced the silent empty-document bug and this fix locally, with the base (4cc9fa1) and head (ad145f0) trees side by side, then ran the new tests, the package suite and the formatter check on both. The fix does what the description says for the layouts it targets; the notes below are about the parts of the nbformat 3 layout that are still dropped, plus two incidental behaviour changes worth pinning with a test.

1. The reported bug reproduces, and the new cell walker fixes it

Each row is a probe notebook converted through IpynbConverter().convert(...) on both trees; len is the produced markdown length.

accepts() does claim an application/json stream whose text contains nbformat and nbformat_minor, so the empty result really was returned as a successful conversion rather than falling through to the text converter — I could not find a path where the plain-text fallback rescued it.

2. A notebook written by nbformat's own v3 writer still loses its heading cells

This is the case I would most like covered before merge, because it is the file an upstream tool actually produces. I built the notebook with the nbformat package itself (v3.new_notebook, v3.new_heading_cell("Quarterly notes", level=1), v3.new_code_cell, v3.new_text_cell("markdown", ...)), wrote it with nbformat.write(nb, file, version=3) and validated it with nbformat.validate:

top-level keys: ['metadata', 'nbformat', 'nbformat_minor', 'worksheets']
  {'cell_type': 'heading', 'level': 1, 'source': ['Quarterly notes']}
  {'cell_type': 'code', 'input': ["print('hello')"], 'outputs': [], 'prompt_number': 1}
  {'cell_type': 'markdown', 'source': ['Revenue grew.\n']}
  {'cell_type': 'code', 'input': ['2 + 2'], 'outputs': [], 'prompt_number': 2}

Converting that file:

base: len=0    title=None
head: len=64   title=None     -> code and markdown cells kept, the heading cell is dropped

The same content upgraded to nbformat 4 with nbformat.reads(raw, as_version=4) gives cell types ['markdown', 'code', 'markdown', 'code'] with the first source '# Quarterly notes', and converting that v4 form yields len=83, title='Quarterly notes'. The 19 missing characters are exactly the heading. The format description is explicit that this is v3-only content: "Heading cells have been removed in favor of simple headings in markdown. Changed in version nbformat: 4.0."

heading is a first-class nbformat 3 cell type ({"cell_type": "heading", "level": 1, "source": ...}), so the loop in _convert() currently falls through all three branches and the cell disappears silently. Two consequences:

• A v3 notebook whose content lives in heading cells still converts to an empty document with exit code 0 — the exact symptom this PR fixes for code and markdown cells. Probe: a single heading cell with source: ["Section A\n", "more text\n"] → head len=0.
• Titles that used to come from the first # line of a markdown cell are often in a heading cell in v3 files, which is why the title is None above.

If you want to keep the change minimal, mapping a heading to markdown text ("#" * cell.get("level", 1) + " " + "".join(source_lines)) would make the v3 path match the v4 result for the fixture above; at minimum it deserves a test so the gap is recorded rather than silent.

3. A schema-valid single-string source/input still loses the title (pre-existing, but adjacent to this change)

The new fallback is deliberately shape-agnostic — "".join(source_lines) works for a string and for a list — but the title extraction right below it is not:

for line in source_lines:
    if line.startswith("# "):

When source_lines is a plain string, this iterates characters, so "# Quarterly notes" never matches and the title is lost. Both shapes are valid on disk; the format description says you "must allow multi-line string fields to be either a string or list of strings". Probe with identical content:

• source: ["# Quarterly notes\n", "\n", "Revenue grew.\n"] → title='Quarterly notes'
• source: "# Quarterly notes\n\nRevenue grew.\n" → title=None (same on the v4 path, base and head, so this is not introduced here)

Normalising once (if isinstance(source_lines, str): source_lines = [source_lines]) would fix the title path and remove the character-iteration subtlety for all cell types.

4. Two incidental robustness gains that the description does not mention

• {"cells": null, "worksheets": [...]} — base raises FileConversionException: Error converting .ipynb file: 'NoneType' object is not iterable; head returns the worksheet content (len=16). The is not None check is what makes the difference; that is a real crash fix worth a line in the description and a test.
• nbformat 2 files use the same worksheets[*].cells + input layout, so they convert now as well (probe: nbformat: 2 code cell → base len=0, head len=26). The docstring says nbformat 3; nbformat 2 works too, untested.

5. One precedence nuance

{"cells": [], "worksheets": [non-empty]} still converts to nothing, because an empty top-level list wins over the fallback, while null falls back. A file with both keys is malformed either way, so this is not a defect — but since the two branches differ, one test pinning the intended precedence (and a comment on the cells is not None line) would keep the next reader from "fixing" it in the other direction.

6. Test, suite and lint evidence

• New file on head: 8 passed. Same file copied into the base tree: 6 failed, 2 passed — exactly the six you list, with test_nbformat_3_title_from_metadata_still_wins and test_nbformat_4_is_unchanged green on both sides.
• Package suite with the same test set on both sides and only the source tree swapped (pytest packages/markitdown/tests, test_cu.py ignored because azure-ai-contentunderstanding is not installed here): base 37 failed, 957 passed, 3 skipped → head 31 failed, 963 passed, 3 skipped. Diffing the FAILED lines gives exactly the six new tests — no other test changes state in either direction.
• The 31 remaining failures are the pre-existing environment ones: test_cli_vectors.py (21), test_cli_misc.py (7), test_docintel.py (2), test_module_misc.py (1).
• Existing ipynb tests in test_module_misc.py (-k ipynb): 6 passed on both sides.
• black 23.7.0 --check (the version pinned in .pre-commit-config.yaml) on _ipynb_converter.py and the new test file: exit 0.
• CI on this head has not run: only license/cla completed; the tests and pre-commit workflow runs are action_required, and the PR shows mergeable_state=blocked. So nothing in §1–§3 was verified by CI — the numbers above are from a local run on Windows, CPython 3.13.12.

7. Out of scope, no regression

Code-cell outputs are ignored for both layouts (v3 stream/pyout shapes never reach the renderer), and the code fence still gets a blank line before the closing fence when the cell text ends in "\n". Both are unchanged by this PR; I mention them only so they are not mistaken for new behaviour.

Disclosure: this review was prepared with an AI coding assistant (DeepSeek) under my direction; every probe, test run and command above was executed locally by me, and the review was posted from my account.